Conversation
67e059c to
8442a35
Compare
There was a problem hiding this comment.
Pull request overview
Work-in-progress updates to epmt_query to support anomaly detection workflows by enabling saving reference models separately from creation and improving DB model management.
Changes:
- Added a new
save_refmodel()helper for persisting reference models. - Updated
create_refmodel()to callsave_refmodel()instead of directly creating ORM objects. - Added a new
test_save_refmodel.pyscript intended to exercise refmodel creation/saving.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| test_save_refmodel.py | Adds a script-like “test” that creates/deletes a refmodel and calls save_refmodel(). |
| src/epmt/epmt_query.py | Alters refmodel creation flow and introduces save_refmodel() for DB persistence. |
| @@ -1357,9 +1361,18 @@ def create_refmodel(jobs, name=None, tag={}, op_tags=[], | |||
| return r.id | |||
| r_dict = orm_to_dict(r, with_collections=True) | |||
| return pd.Series(r_dict) if fmt == 'pandas' else r_dict | |||
|
|
|||
| ''' | |||
There was a problem hiding this comment.
create_refmodel() no longer returns the created reference model (the prior return logic is now inside a triple-quoted string). This is a behavior-breaking change: callers will now receive None instead of the created model/id/dict. Make create_refmodel() return the result of save_refmodel(...) (and remove the triple-quoted block rather than leaving it as a stray string literal statement).
| def save_refmodel(ReferenceModel, jobs, computed, info_dict, enabled, name=None, tags={}, op_tags=[], fmt='dict'): | ||
| r = orm_create(ReferenceModel, jobs=jobs, computed=computed, info_dict = info_dict, enabled=enabled, name=name, tags=tags, op_tags=op_tags, fmt=fmt) | ||
| orm_commit() | ||
| if fmt == 'orm': | ||
| return r | ||
| elif fmt == 'terse': | ||
| return r.id | ||
| r_dict = orm_to_dict(r, with_collections=True) | ||
| return pd.Series(r_dict) if fmt == 'pandas' else r_dict |
There was a problem hiding this comment.
A few API/implementation issues in save_refmodel():\n- It uses mutable default arguments (tags={}, op_tags=[]), which can leak state between calls. Use None defaults and assign inside the function.\n- The first parameter is named ReferenceModel, which reads like the ORM class rather than an argument; it’s clearer as model_cls (or omit it and reference ReferenceModel directly if it’s always the same).\n- Consider decorating with @db_session (like create_refmodel) so save_refmodel() is safe to call directly outside an existing session.\n- orm_create(..., fmt=fmt) is potentially problematic if orm_create doesn’t accept fmt (the prior code handled formatting after orm_create). If orm_create doesn’t support it, remove fmt=fmt and keep formatting in save_refmodel().
|
|
||
| feature_list = [ 'duration', 'rchar', 'syscr', 'syscw', 'wchar', 'cstime', 'cutime', 'majflt', 'cpu_time', 'minflt', 'rssmax', 'cmajflt','cminflt', 'inblock', 'outblock', 'usertime', 'num_procs', 'starttime', 'vol_ctxsw', 'read_bytes', 'systemtime', 'time_oncpu', 'timeslices', 'invol_ctxsw', 'write_bytes', 'time_waiting', 'cancelled_write_bytes'] | ||
| random_jobs = eq.get_jobs(limit = 100, fmt = 'dict', trigger_post_process=False) | ||
|
|
||
| try: | ||
| r = eq.get_refmodels("bronx_test_model") | ||
| eq.delete_refmodels(r[0]['id']) | ||
| finally: | ||
| print("ready for new bronx model") | ||
| r = eq.create_refmodel(random_jobs, methods=es.mvod_classifiers(), features = feature_list, name = "bronx_test_model") | ||
|
|
||
| eq.save_refmodel(ReferenceModel, r['jobs'], r['computed'], r['info_dict'], r['enabled'], name='test_name', tag={}, op_tags=[]) |
There was a problem hiding this comment.
This file is named like a unit test but behaves like a destructive integration script: it performs DB reads/writes, deletes existing models, and has no assertions. This will be flaky and potentially harmful in CI/dev environments.\n\nSuggested direction (pick one):\n- Convert it to a real automated test (e.g., pytest) with assertions and isolated DB/fixtures (and avoid deleting arbitrary existing models), or\n- Move it to a scripts/ or examples/ location and keep it out of the test discovery path.\n\nAlso, the call uses tag={} but save_refmodel() expects tags=... (per its signature).
| feature_list = [ 'duration', 'rchar', 'syscr', 'syscw', 'wchar', 'cstime', 'cutime', 'majflt', 'cpu_time', 'minflt', 'rssmax', 'cmajflt','cminflt', 'inblock', 'outblock', 'usertime', 'num_procs', 'starttime', 'vol_ctxsw', 'read_bytes', 'systemtime', 'time_oncpu', 'timeslices', 'invol_ctxsw', 'write_bytes', 'time_waiting', 'cancelled_write_bytes'] | |
| random_jobs = eq.get_jobs(limit = 100, fmt = 'dict', trigger_post_process=False) | |
| try: | |
| r = eq.get_refmodels("bronx_test_model") | |
| eq.delete_refmodels(r[0]['id']) | |
| finally: | |
| print("ready for new bronx model") | |
| r = eq.create_refmodel(random_jobs, methods=es.mvod_classifiers(), features = feature_list, name = "bronx_test_model") | |
| eq.save_refmodel(ReferenceModel, r['jobs'], r['computed'], r['info_dict'], r['enabled'], name='test_name', tag={}, op_tags=[]) | |
| import uuid | |
| feature_list = [ 'duration', 'rchar', 'syscr', 'syscw', 'wchar', 'cstime', 'cutime', 'majflt', 'cpu_time', 'minflt', 'rssmax', 'cmajflt','cminflt', 'inblock', 'outblock', 'usertime', 'num_procs', 'starttime', 'vol_ctxsw', 'read_bytes', 'systemtime', 'time_oncpu', 'timeslices', 'invol_ctxsw', 'write_bytes', 'time_waiting', 'cancelled_write_bytes'] | |
| def test_save_refmodel_persists_refmodel(): | |
| """ | |
| Integration-style test that creates a reference model, saves it, | |
| verifies it can be retrieved, and then cleans it up. | |
| """ | |
| model_name = "bronx_test_model_" + uuid.uuid4().hex | |
| random_jobs = eq.get_jobs(limit=100, fmt='dict', trigger_post_process=False) | |
| r = eq.create_refmodel( | |
| random_jobs, | |
| methods=es.mvod_classifiers(), | |
| features=feature_list, | |
| name=model_name, | |
| ) | |
| try: | |
| eq.save_refmodel( | |
| ReferenceModel, | |
| r['jobs'], | |
| r['computed'], | |
| r['info_dict'], | |
| r['enabled'], | |
| name=model_name, | |
| tags={}, | |
| op_tags=[], | |
| ) | |
| saved_models = eq.get_refmodels(model_name) | |
| assert saved_models, "Expected at least one saved reference model" | |
| finally: | |
| # Clean up only the models created for this test | |
| try: | |
| models_to_delete = eq.get_refmodels(model_name) | |
| except Exception: | |
| models_to_delete = [] | |
| for m in models_to_delete: | |
| if 'id' in m: | |
| eq.delete_refmodels(m['id']) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
26df9c3 to
1281d9f
Compare
Describe your changes
Work-in-progress changes to epmt_query designed to enable anomaly detection. Focus on allowing saving of models separate from creation, active editing of already created models, and management of models in DB.
Issue ticket number, link (if applicable)
Checklist
A loose guide to provide structure for contributions